Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: add tests workflow #120

Merged
merged 9 commits into from
May 10, 2024
Merged

ci: add tests workflow #120

merged 9 commits into from
May 10, 2024

Conversation

ee7
Copy link

@ee7 ee7 commented Feb 19, 2024

Add a workflow that runs nimble build and test.

The tests currently fail, so make the workflow indicate success even if the tests fail for now. This workflow is still useful because it ensures that nimble build succeeds - that command errored on Linux until recently (#128).

Closes #121


To-do:

  • Consider either:
    • requiring a PR to push to the jtv/v2 branch
    • or running this tests workflow on push to jtv/v2
  • Consider not running on push to dev if we won't use that branch in the future

@ee7 ee7 force-pushed the ee7/ci-add-tests-workflow branch 2 times, most recently from e68fafb to 2b24f18 Compare February 19, 2024 15:10
@ee7 ee7 changed the base branch from dev to jtv/v2 February 19, 2024 15:10
@ee7 ee7 force-pushed the ee7/ci-add-tests-workflow branch 3 times, most recently from 267cd8d to 8200a9e Compare April 2, 2024 13:04
@ee7 ee7 changed the base branch from jtv/v2 to ee7/fix-nimble-build-on-linux April 2, 2024 13:17
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@ee7 ee7 requested a review from miki725 April 2, 2024 13:20
run: nimble build

- name: Run tests
run: ./test || exit 0 # TODO: Stop allowing tests to fail.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do tests fail now? Even if they do I think we can still merge prs until we fix all tests

Copy link
Author

@ee7 ee7 Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, tests do fail now.

I'm OK with removing the || exit 0 and not requiring a PR that targets jtv/v2 in this repo to pass all checks before merging - then the check status actually reflects the tests. However, currently the branch protection rule in this repo blocks this PR being merged.

Base automatically changed from ee7/fix-nimble-build-on-linux to jtv/v2 April 8, 2024 15:29
ee7 added 5 commits April 8, 2024 18:37
For now, make the workflow indicate success even if the tests fail. This
workflow still asserts that `nimble build` succeeds, which it didn't
until now on Linux.
This reverts the previous commit.

On macOS x86_64, the workflow errored with:

    clang: error: no such file or directory: '/Users/runner/.local/c0/libs/libffi.a'

and on macOS arm64 it errored with:

    ERROR: unable to find archive matching pattern macosx_arm64.tar.xz

because the nim nightlies repo doesn't have binary builds for that platform.
@ee7 ee7 force-pushed the ee7/ci-add-tests-workflow branch from 8200a9e to 683db8f Compare April 8, 2024 16:38
@ee7 ee7 marked this pull request as ready for review April 8, 2024 16:43
@ee7 ee7 requested a review from viega as a code owner April 8, 2024 16:43
@ee7 ee7 requested a review from miki725 April 8, 2024 16:43
ee7 added 3 commits May 10, 2024 15:32
Bump to the latest release. See the new commits [1].

[1] actions/checkout@v4.1.1...v4.1.4
Bump to the latest release. See the new commits [1].

[1] iffy/install-nim@v5.0.3...v5.0.4
Be consistent with version comments in our workflows elsewhere.
@ee7
Copy link
Author

ee7 commented May 10, 2024

@miki725 Following up from #120 (comment) and later discussion, our preference was for us to disable/bypass the branch protection rule that requires checks to succeed, and merge this PR as-is, right? If so, could you please disable the branch protection rule for jtv/v2 for now? Then I'll merge this.

@miki725
Copy link
Contributor

miki725 commented May 10, 2024

v2 is not a protected branch. as such there are no merge requirements here

@ee7
Copy link
Author

ee7 commented May 10, 2024

v2 is not a protected branch. as such there are no merge requirements here

Well, I have write permissions in this repo, and this PR's branch is strictly ahead of jtv/v2. But I was unable to merge this PR before your approval, and I'm still unable to merge this PR now (even after Ctrl + F5). Do you know why?

@ee7
Copy link
Author

ee7 commented May 10, 2024

Oh, sorry. I didn't notice that I could merge this - I guess I'm much too conditioned to needing the button to go green first.

1

@ee7 ee7 merged commit 6d1f3bd into jtv/v2 May 10, 2024
1 of 2 checks passed
@ee7 ee7 deleted the ee7/ci-add-tests-workflow branch May 10, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants